feat: max-length config
#194
Conversation
rocktimsaikia
force-pushed
the
max-length-flag
branch
from
11938f7
to
6e34e8b
Compare
src/utils/config.ts
Outdated
| } | ||
|
|
||
| parseAssert('length', /^\d+$/.test(length), 'Must be an integer'); | ||
|
|
||
| const parsed = Number(length); | ||
| parseAssert('length', parsed >= 5, 'Must be greater than 5 tokens(20 characters))'); | ||
| parseAssert('length', parsed >= 20, 'Must be greater than 20 characters'); |
There was a problem hiding this comment.
This can change. You can decide the minimum limit. 20 seems alright to me.
rocktimsaikia
force-pushed
the
max-length-flag
branch
from
c2a76a6
to
9f5e8c4
Compare
rocktimsaikia
force-pushed
the
max-length-flag
branch
from
9f5e8c4
to
2ab697c
Compare
| @@ -100,18 +97,45 @@ const sanitizeMessage = (message: string) => message.trim().replace(/[\n\r]/g, ' | |||
|
|
|||
| const deduplicateMessages = (array: string[]) => Array.from(new Set(array)); | |||
|
|
|||
| const getPrompt = (locale: string, diff: string) => `Write a git commit message in present tense for the following diff without prefacing it with anything. Do not be needlessly verbose and make sure the answer is concise and to the point. The response must be in the language ${locale}:\n${diff}`; | |||
| const getPrompt = (locale: string, diff: string, length: number) => `Write a git commit message in present tense for the following diff without prefacing it with anything. Do not be needlessly verbose and make sure the answer is concise and to the point. The response must be no longer than ${length} characters. The response must be in the language ${locale}:\n${diff}`; | |||
There was a problem hiding this comment.
Should we pad it by 5 in the prompt limit(${length}) too? RN we are only padding it in char to token conversion. But padding here might result in 50+ characters in a commit message eventually.
|
src/utils/openai.ts
Outdated
| @@ -1,7 +1,8 @@ | |||
| import https from 'https'; | |||
| import type { ClientRequest, IncomingMessage } from 'http'; | |||
| import type { CreateChatCompletionRequest, CreateChatCompletionResponse } from 'openai'; | |||
| import { type TiktokenModel } from '@dqbd/tiktoken'; | |||
| // eslint-disable-next-line camelcase | |||
| import { TiktokenModel, encoding_for_model } from '@dqbd/tiktoken'; | |||
There was a problem hiding this comment.
Can you break up each named import into its own line so the eslint disable directive can apply specifically to encoding_for_model?
There was a problem hiding this comment.
Doing that will result in import/no-duplicates error
There was a problem hiding this comment.
This is what I mean:
import {
TiktokenModel,
// eslint-disable-next-line camelcase
encoding_for_model,
} from '@dqbd/tiktoken';
src/utils/openai.ts
Outdated
| // Padded by 5 for more room for the completion. | ||
| const stringFromLength = generateStringFromLength(length + 5); | ||
| const tokenFromLength = getTokens(stringFromLength, model); | ||
| const tokenFromPrompt = getTokens(prompt, model); |
There was a problem hiding this comment.
Is it possible to concatenate prompt + stringFromLength and pass it into getTokens once?
That way you can remove the getTokens util.
| for (let i = 0; i < length; i += 1) { | ||
| const randomIndex = Math.floor(Math.random() * characters.length); | ||
| result += characters.charAt(randomIndex); | ||
| } |
There was a problem hiding this comment.
What's the highest token character? I think we can just use that instead of generating a random value every time.
|
@privatenumber The changes are done. Please have a look |
|
@privatenumber ICYMI |
|
Haven't forgotten (just backed up right now), but thanks for the reminder. |
|
Going to rename this to |
|
That's alright 👍 |
|
I think the test keeps failing because max-length sets a default length of 50, and that's shared for multiple results: Do you think you can open a PR to multiply it for the number of options to generate? |
|
Okey I will look into that |
|
Actually, based on the docs I think |
fixes #184 #91
closes #120
Setting the default to 50 characters.